Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accessibility: Handle the iOS z-gesture to exit modals and block selection #926

Merged
merged 18 commits into from
May 23, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Apr 24, 2019

Gutenberg PR: WordPress/gutenberg#15153

This PR improves accessibility on modals and blocks. It also makes images easily editable by listening for a long press event on the image.

Testing Instructions (iOS)

  • Activate VoiceOver (Settings > General > Accessibility > VoiceOver) or say "Hey Siri, activate VoiceOver"
  • Boot the demo app on iOS, go from block to block by swiping on the right
  • Notice the screen reader says "Image block. Empty. Button. Double tap to select an image" for image blocks without an image
  • Notice the screen reader says "Image block. Button" for non empty image block
  • Double tap on a selected non-empty image block
  • Notice it says "Image block. Long press to edit the image. button image."
  • Go to the next item and notice it says "Image caption. Empty. Button. Actions available"
  • Go back (swipe left) to the image and notice it says "Long press to edit the image. button image."
  • Long press the image (when VoiceOver is active you may have to tap once first, it's like a double tap but on the second tap your finger stays on the screen, you will hear 3 sounds in this case) until the screen to edit the image pops up. Edit the image if you want to
  • Do the z-gesture (make a z with 2 figers on the screen) and notice the currently selected block is unselected
  • Try to close other modals with the z-gesture

Testing Instructions (Android)

  • Activate TalkBack (Settings > Accessibility > TalkBack)
  • Boot the demo app on Android, go from block to block by swiping on the right
  • Run the same tests as on iOS (ignore the fact that the block container is accessible at the moment)
  • Notice that by swiping right you enter inside the block (RN ignores the fact that an element is a child of an accessible element)
  • Notice the z-gesture does not work for unselecting a block
  • Notice that the double long tap works to edit the image but works for modals

@pinarol pinarol modified the milestones: v1.4, v1.5 May 3, 2019
@etoledom
Copy link
Contributor

Updated from develop to incorporate changes made from the Video Block PR.

There was a problem to implement long press on the image to show the image edit options:
Since those options were extracted to the MediaUpload component, the function to show the picker is not in scope anymore.

I fixed this problem simply by removing the long-press gesture from the image, but this is not ideal.

Thinking on a way to preserve the modularization of MediaUpload and also have access to present the picker from anywhere in the ImageEdit component, I thought on trying to implement MediaUpload as a HOC, and pass the presentMediaOptions as a prop.

I did this in this branch: https://github.com/WordPress/gutenberg/compare/rnmobile/media-upload-as-hoc?w=1

It turned out to be a big-ish refactor, so I prefer to not add it to this PR.
I also don't like the fact that MediaUpload is now a HOC, and with this we are going away from the web implementation of that component. But I'm not sure where else to add it.

If there's a better place, or a better alternative, I'd be happy to edit that branch.

About this PR, I think that to not add the long-press functionality just yet is the best option for now.
@Tug , @pinarol could you check this out whenever you have a sec please?
Thank you!

@Tug Tug modified the milestones: v1.5, v1.6 May 16, 2019
@pinarol
Copy link
Contributor

pinarol commented May 16, 2019

hey @etoledom if I understood correctly what you are trying to do is opening the MediaUpload options picker on a long press. You can do that just by wrapping the component with <MediaUpload />

<MediaUpload
	render={ ( { open, getMediaOptions } ) => {
                { getMediaOptions() }
                <TheComponent onLongPress= { open } />
        }
/>

I don't think we should move that much away from the web side, it would make it very hard to implement media related blocks cross platform in the future.

@etoledom
Copy link
Contributor

You can do that just by wrapping the component with

So, it's fine to wrap all the MediaUploadProgress component plus the BlockControls bits inside MediaUpload's render prop?

I thought about that for a second but it sounds a bit messy 🤔
I'll give it a try anyway.

I don't think we should move that much away from the web side

I totally agree.

@pinarol
Copy link
Contributor

pinarol commented May 16, 2019

@etoledom as far as I understand it is enough to wrap these two separately:

Screen Shot 2019-05-16 at 16 22 13

--------

Screen Shot 2019-05-16 at 16 22 17

@etoledom
Copy link
Contributor

@pinarol - Thank you for the recommendations!

Updated with the recommended solution.

I noticed that the longPress prop on ImageBackground is not needed (and it also doesn't work).
Still the access to show the ActionSheet from the root component was needed.

Tried to do it in a way that the syntax doesn't look so bad, converting the current component into a function, and returning that function on MediaUpload's render prop:

https://github.com/WordPress/gutenberg/blob/12ff491ac2243c05ba0858ec66c4cb1316fd3bba/packages/block-library/src/image/edit.native.js#L383-L390

.vscode/settings.json Outdated Show resolved Hide resolved
@pinarol
Copy link
Contributor

pinarol commented May 21, 2019

@etoledom thanks, looks good 👍 let me test a bit

@etoledom etoledom self-assigned this May 21, 2019
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with WPiOS, looks and works good 🎉

@Tug Tug merged commit d8c5082 into develop May 23, 2019
@Tug Tug deleted the update/accessibility-ios-z-gesture branch May 23, 2019 15:24
@etoledom
Copy link
Contributor

Thank you @pinarol for the review and @Tug for the update and merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants